Do not write a proxy's port into a forwarded https url - #8
Merged
Conversation
asUrl() resolved the scheme twice, and differently. The output used
isSecure(true), which honours X-Forwarded-Proto, while the port check was
handed getScheme(), which does not. Behind a tls terminating proxy the two
disagreed, so the url claimed https while port suppression was evaluated as
http, leaving the origin's port in place:
Request::create('http://www.packaged.local:443/') + X-Forwarded-Proto
-> https://www.packaged.local:443
Passing the resolved scheme to _isStandardPort() alone is not enough. A proxy
forwards on its own port, which is usually neither 80 nor 443 -- 8080 is the
common case -- so no default-port comparison suppresses it. The port the
request arrived on simply is not part of the public url.
So when the scheme came from the forwarded header and no port was set
explicitly, the port is now omitted. An explicit setPort() still wins, and
requests that were not proxied are unaffected.
Note this assumes the public url is on the scheme's default port, which is
all X-Forwarded-Proto tells us. Configure trusted proxies if the public port
is non-standard; getPort() then honours X-Forwarded-Port.
The existing forwarded-proto assertion only passed because it used port 80,
which is standard for the mis-resolved http scheme. Adds the 443 case, which
fails without this change.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the note on #7.
asUrl()resolved the scheme twice, differently: the output usedisSecure(true), which honoursX-Forwarded-Proto, while the port check was handedgetScheme(), which doesn't. Behind a TLS-terminating proxy they disagreed, so the URL claimedhttpswhile port suppression was evaluated ashttp:Passing the resolved scheme to
_isStandardPort()isn't sufficient, which is worth flagging since it's the obvious one-line fix — I tried it and it broke the existing forwarded-proto assertion. A proxy forwards on its own port, usually neither 80 nor 443 (8080 being the common case), so no default-port comparison suppresses it. The port the request arrived on simply isn't part of the public URL.So when the scheme came from the forwarded header and no port was set explicitly, the port is omitted. Explicit
setPort()still wins; non-proxied requests are untouched.This assumes the public URL is on the scheme's default port, which is all
X-Forwarded-Prototells us. If the public port is non-standard, configure trusted proxies —getPort()then honoursX-Forwarded-Port.Test plan
The existing forwarded-proto assertion only passed because it used port 80, standard for the mis-resolved
httpscheme. Added the 443 case; it fails without the src change, which I confirmed before fixing.Full suite green on PHP 8.2.33 and 8.5.10: 45 tests, 161 assertions, exit 0.
Also checked by hand, since tests don't cover all of it:
:8080https://host:80/:443https://hostsetPort(8443)https://host:8443:81/:80http://host:81/http://host:443/:8443https://host/https://host:8443setScheme('https'), request:81https://host:81🤖 Generated with Claude Code